BlockStoreProtocol #19799

richardkiss · 2025-07-10T00:17:30Z

Remove a dependency of chia.full_node from chia.consensus.

coveralls-official · 2025-07-10T01:01:55Z

Pull Request Test Coverage Report for Build 16382258510

Details

59 of 60 (98.33%) changed or added relevant lines in 7 files are covered.
15 unchanged lines in 8 files lost coverage.
Overall coverage increased (+0.004%) to 91.299%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
chia/full_node/block_store.py	15	16	93.75%

Files with Coverage Reduction	New Missed Lines	%
chia/full_node/full_node.py	1	88.24%
chia/full_node/pending_tx_cache.py	1	96.55%
chia/server/node_discovery.py	1	82.29%
chia/wallet/util/wallet_sync_utils.py	1	86.07%
chia/full_node/full_node_api.py	2	85.45%
chia/timelord/timelord_launcher.py	2	90.71%
chia/wallet/wallet_node.py	3	87.6%
chia/server/server.py	4	82.51%

Totals
Change from base Build 16375210539:	0.004%
Covered Lines:	101935
Relevant Lines:	111530

💛 - Coveralls

richardkiss · 2025-07-17T21:57:52Z

chia/_tests/blockchain/blockchain_test_utils.py


 async def check_block_store_invariant(bc: Blockchain):
-    db_wrapper = bc.block_store.db_wrapper
+    block_store = cast(BlockStore, bc.block_store)


This is ugly for sure, but we can get away with it because we know that bc has been populated with a BlockStore instance here.

In future PRs, I expect I will have to rework some tests pretty significantly.

richardkiss · 2025-07-17T21:59:08Z

chia/_tests/blockchain/blockchain_test_utils.py

    in_chain = set()
    max_height = -1
-    async with db_wrapper.writer_maybe_transaction() as conn:
+    async with block_store.transaction() as conn:


The BlockStoreProtocol now has a .transaction generic async context manager which for BlockStore does the same thing as the prior code.

richardkiss · 2025-07-17T22:00:24Z

chia/_tests/blockchain/test_blockchain.py

            await _validate_and_add_block(b, block)

-        blocks = await b.get_block_records_at(heights, batch_size=2)
+        blocks = await b.get_block_records_at(heights)


The batching should be handled by the BlockStoreProtocol instance. I've pushed it into BlockStore, so get_block_records_at no longer accepts batch_size (which is how it really should work, right?).

richardkiss · 2025-07-17T22:05:01Z

chia/consensus/blockchain.py

        try:
            # Always add the block to the database
-            async with self.block_store.db_wrapper.writer():
+            async with self.block_store.transaction():


We don't want to require that our BlockStoreProtocol instance have a very sqlite3-specific .db_wrapper (since Rocks DB won't).

we currently rely on BlockStore and CoinStore to use the same database and that updates to them are part of the same transaction. It's not great that this dependency is kind of implied in many ways. But I think it's important to preserve. If we extend the chain by one block in BlockStore and that transaction commits, the CoinStore better have the new coins committed too, as well as the state of the coin set (which is currently implied by the coin table).

This looks like a step away from this, and it's not clear to me that we can step away from it without some other mechanism to keep them in sync.

Don't worry, I have this issue firmly in my sights. As we discussed elsewhere, DBWrapper2 is a problem because it requires all stores to have the same DB, which may not continue to be true going forward. But I am aware of why transactional integrity is required and am keeping it in mind. A two-phase commit with resolution of conflicts upon DB open is in our future (if necessary).

richardkiss · 2025-07-17T22:05:43Z

chia/consensus/blockchain.py

        blocks: list[FullBlock] = []
        for hash in hashes.copy():
-            block = self.block_store.block_cache.get(hash)
+            block = self.block_store.get_block_from_cache(hash)


We don't want to require our BlockStoreProtocol to have a block_cache. Ultimately, we should consider unifying the block cache and BlockStoreProtocol.

yeah, I agree. I experimented with making the cache transparent once, and there are a lot of functions that would have to turn async and probably slow down a lot as a result

richardkiss · 2025-07-17T22:06:16Z

chia/consensus/blockchain.py

        return header_dict[header_hash]

-    async def get_block_records_at(self, heights: list[uint32], batch_size: int = 900) -> list[BlockRecord]:
+    async def get_block_records_at(self, heights: list[uint32]) -> list[BlockRecord]:


We don't actually want a batch_size parameter here: it should be an implementation detail.

richardkiss · 2025-07-17T22:06:48Z

chia/consensus/blockchain.py

-            records.extend(res)
-        return records
+
+        return await self.block_store.get_block_records_by_hash(hashes)


This function now deals with the batching on its own.

richardkiss · 2025-07-17T22:07:25Z

chia/full_node/block_store.py

-                for row in await cursor.fetchall():
-                    block_rec = BlockRecord.from_bytes(row[1])
-                    all_blocks[block_rec.header_hash] = block_rec
+        for batch in to_batches(header_hashes, self.db_wrapper.host_parameter_limit):


The batching is now done here.

richardkiss · 2025-07-17T22:08:58Z

chia/full_node/full_node_api.py


        if request.end_height < request.start_height or request.end_height - request.start_height > 128:
            return make_msg(ProtocolMessageTypes.reject_block_headers, reject)
-        if self.full_node.block_store.db_wrapper.db_version == 2:


We now always try the new get_block_bytes_in_range API and only fall back to the version 1 api if we get a NotImplementedError exception

richardkiss · 2025-07-17T22:09:33Z

tach.toml

 depends_on = [
    "chia.types",
    "chia.util",
-    { path = "chia.full_node", deprecated = false },


The chia.consensus module no longer depends upon chia.full_node. Hooray!

Copilot

Pull Request Overview

This PR introduces a BlockStoreProtocol to remove the dependency of chia.consensus on chia.full_node, allowing for better separation of concerns and cleaner architecture.

Creates a new protocol interface BlockStoreProtocol that defines the required block store methods
Refactors the blockchain module to use the protocol instead of the concrete BlockStore class
Updates block store implementation to include optimized database query batching and improved error handling

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tach.toml	Removes the deprecated dependency from chia.consensus to chia.full_node
chia/consensus/block_store_protocol.py	Introduces the new BlockStoreProtocol interface
chia/consensus/blockchain.py	Updates to use BlockStoreProtocol and removes batching logic
chia/full_node/block_store.py	Implements protocol methods and adds database query optimizations
chia/full_node/full_node_api.py	Improves exception handling for block store operations
chia/simulator/full_node_simulator.py	Updates to use the new transaction method
chia/_tests/blockchain/test_blockchain.py	Removes batch_size parameter from test
chia/_tests/blockchain/blockchain_test_utils.py	Adds type casting for concrete BlockStore usage

Comments suppressed due to low confidence (1)

Copilot · 2025-07-18T19:45:34Z

chia/full_node/full_node_api.py

+            blocks_bytes = await self.full_node.block_store.get_block_bytes_in_range(
+                request.start_height, request.end_height
+            )
+        except NotImplementedError:


The exception handling structure has a logical issue. The except NotImplementedError block contains code that can raise ValueError, but the except ValueError handler is placed after this block, making it unreachable for ValueErrors raised within the NotImplementedError handler.

arvidn · 2025-07-29T20:17:14Z

chia/consensus/block_store_protocol.py

+        self, blocks_n: int
+    ) -> tuple[dict[bytes32, BlockRecord], Optional[bytes32]]: ...
+    async def get_prev_hash(self, header_hash: bytes32) -> bytes32: ...
+    def transaction(self) -> AbstractAsyncContextManager[Any]: ...


is there a good reason to use Any instead of aiosqlite.Connection as the type parameter here?
Doing so would preserve some type safety, and make it easier to refactor (with confidence) into a generic type in the future, when such type exists.

Yeah, right now, the only concrete implementation of this protocol returns a aiosqlite.Connection, but it's not actually used anywhere except in tests. I think full_node rather just goes to the DBWrapper2 to start a transaction. So yeah, this is pretty useless and dumb which is why I think my next step should really be a ConsensusStoreProtocol to abstract away the DBWrapper2/sqlite3 stuff. Mostly I just didn't want consensus to have to depend on aiosqlite since it's just a detail.

I think ConsensusStore should actually have an async context manager that returns a thing that you actually do the writes to, and that can have a non-trivial type (although it will be a protocol too).

I agree it's dumb. The main goal of this PR is to remove chia.full_node as a dep of chia.consensus. Once that's done, I can do some better refactoring of storage stuff around a new ConsensusStoreProtocol. Because of the layer violation that is DBWrapper2 (which forces all stores to be in the same sqlite3 DB), it's kind of hard to type this in a useful way.

it almost seems that those things ought to happen in the opposite order. to avoid Any and cast() as a temporary measure in between

I want to remove the circular dependencies between chia.consensus and chia.full_node first so I can be assured that chia.full_node can't interfere with changes to the chia.consensus interface.

This reverts commit f077801.

github-actions · 2025-09-16T11:06:21Z

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

richardkiss requested a review from a team as a code owner July 10, 2025 00:17

richardkiss marked this pull request as draft July 10, 2025 00:17

richardkiss added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes labels Jul 10, 2025

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 12, 2025

richardkiss force-pushed the block_store_protocol branch from 806414d to 208c082 Compare July 12, 2025 01:53

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 12, 2025

richardkiss force-pushed the block_store_protocol branch 4 times, most recently from 7cd180b to 1312c4f Compare July 16, 2025 23:17

richardkiss commented Jul 17, 2025

View reviewed changes

richardkiss force-pushed the block_store_protocol branch from ba1f6d6 to 52c49e1 Compare July 17, 2025 22:17

richardkiss marked this pull request as ready for review July 17, 2025 22:45

richardkiss requested a review from Copilot July 18, 2025 19:45

Copilot AI reviewed Jul 18, 2025

View reviewed changes

richardkiss force-pushed the block_store_protocol branch 3 times, most recently from 8372d0d to e09d082 Compare July 18, 2025 23:31

richardkiss force-pushed the block_store_protocol branch from e09d082 to 8047450 Compare July 28, 2025 20:52

arvidn reviewed Jul 29, 2025

View reviewed changes

richardkiss force-pushed the block_store_protocol branch from 8047450 to b92bedf Compare July 30, 2025 20:51

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 1, 2025

richardkiss added 15 commits August 1, 2025 14:44

BlockStoreProtocol

49422b7

Deal with .db_wrapper

165f628

mypy

8046b01

ruff

67986f9

ruff

edb6d3f

mypy

f1f195e

ruff

f1a3dc0

minimize diffs

6a4f2a1

mindiffs

2458f01

Move batching and simplify protocol.

7fbc2f7

Move GeneratorBlockInfo.

6a92c7a

Simplify protocol

6cee193

Use BlockStore locally, for now

2c7c884

Revert "Move GeneratorBlockInfo."

4ac3d58

This reverts commit f077801.

Improve exceptions

2c71a1f

richardkiss force-pushed the block_store_protocol branch from b92bedf to 2c71a1f Compare August 1, 2025 21:49

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Aug 1, 2025

fixup

c97a282

github-actions bot added the stale-pr Flagged as stale and in need of manual review label Sep 16, 2025

BlockStoreProtocol #19799

Are you sure you want to change the base?

BlockStoreProtocol #19799

Conversation

richardkiss commented Jul 10, 2025

Uh oh!

coveralls-official bot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16382258510

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardkiss Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coveralls-official bot commented Jul 10, 2025 •

edited

Loading

richardkiss Jul 17, 2025 •

edited

Loading